-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
topdown/crypto: Add URIStrings field to JSON certs #6424
topdown/crypto: Add URIStrings field to JSON certs #6424
Conversation
70a3a87
to
1f723a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charlieegan3 it would be helpful if we could explain the need for this change in the commit message body. Thanks!
topdown/crypto.go
Outdated
} | ||
} | ||
|
||
v, err := ast.InterfaceToValue(processedCerts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd probably need something similar for crypto.x509.parse_and_verify_certificates
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I've updated the implementation of this too in f71330c
topdown/crypto.go
Outdated
// add a field to certs containing the URIs as strings | ||
processedCerts := make([]struct { | ||
x509.Certificate | ||
RawURIs []string `json:"RawURIs"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment about naming: I think the "raw" in RawURIs
might give the impression that these are presented exactly as they appear in the subjectAltName extension. However, in general, passing a string through url.Parse
and then URL.String
will not always result in the original string.1
What do you think about naming this field URIStrings
instead? (Or something else that doesn't have "raw" in the name?)
Footnotes
-
Here are a few pathological examples: https://go.dev/play/p/I3cnmN5W6dy. It's possible that this shouldn't ever matter if the subjectAltName extension conforms to all the requirements of RFC 5280 4.2.1.6, but I don't really understand this well enough to say for sure. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, I've updated the field name in f71330c
18a2c11
to
f71330c
Compare
f71330c
to
27daf83
Compare
e09a139
to
f5363d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
test/cases/testdata/cryptox509parsecertificates/test-cryptox509parsecertificates-raw-uris.yaml
Show resolved
Hide resolved
test/cases/testdata/cryptox509parsecertificates/test-cryptox509parsecertificates-raw-uris.yaml
Show resolved
Hide resolved
7f377bd
to
a0cd9dd
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
a0cd9dd
to
ce62b35
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
cf27335
to
08bee46
Compare
This is being added to make it easier to write policy on the contents of certificate URI SANs. This is where information like SPIFFE IDs etc are contained and it's helpful to Rego authors to have access to these values without rebuilding the URI from the parsed data under URIs. Fixes open-policy-agent#6416 Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Also, add a test for missing URI case. Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
08bee46
to
3805e5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #6416, see issue for discussion.